Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[배의진] week13 #433

Merged

Conversation

BaeUiJin
Copy link

@BaeUiJin BaeUiJin commented May 17, 2024

작업내용

  • 목적이 같은 커밋들을 합쳐서 다시 정리하였습니다. (Git Kraken 사용)
  • camelCase 로 변수, 함수 표기법을 통일했습니다.
  • React.FC<>, React.PropsWithChildren<> 등 React 타입을 필요한 곳에 지정해주었습니다.
  • 코멘트에도 TODO, QUESTION, NOTE 등 suffix 를 적용하였습니다.

image

멘토님께

이번 주에 새로 만든 커밋은 가장 최근 것 2개가 전부입니다. 그런데 PR 요청을 하고 보니, 해당 커밋 2개 뿐만이 아니라 모든 커밋이 다 올라와 있습니다. '바뀐 파일' 갯수도 173개나 되구요.

이번 주에 새로 만든 브랜치인 'part3-배의진-week13' 위에는 가장 최신 커밋 2개만 있고, 이를 같은 이름의 리모트 브랜치로 push 했습니다. 그리고 그것을 원본 레포지토리의 'part3-배의진' 이란 '새로운' 브랜치로 PR 요청을 올렸습니다. 이 브랜치로 PR 요청을 하는 것은 이번이 처음입니다. 그래서 그런걸까요? Github 에서 PR 요청을 할 때 commit 과 Fileds Changed 를 산정하는 기준을 잘 모르겠습니다.

image

withyj-codeit and others added 30 commits September 3, 2023 21:57
…ithub-actions

[Fix] delete merged branch github action
executing git status command, korean file names were translated into unicode.

thus, unable to identify file with its name.

solution : renamed into english. problem solved
changed file names of hyperlinks of href property as files were renamed into english
tag selectors are less specific, harder to debug than class selectors.

thus, reassigned buttons of signin and signup page with class selectors.
wrong image file has been placed as img-main.png, thus replaced with
right one.

plus, changed image type from png to svg to solve clearity issue when
image size get small.

"head" class's width is adjusted to the same one with "content" class's
width for consistency. 100% to 1050px.
image size, position is changed as instruction.
'box-sizing'option set as 'content-box' and applied globally.
images imported at <div> tag via CSS style rather than <img> tag to fix
its size.
To align items center of the browser, applied 'flex' instead of 'margin'
To align element at the center of the page, applied 'flex' instead of
'margin'
1.in 'head-image' class tag, replaced 'padding-top' with 'aspect-ratio' to make
  code right.

2.in 'contents' class tag, changed order of elements with 'order'
  property.

3.fixed height issue of the 'info' class tag in the 'footer'tag.
BaeUiJin and others added 24 commits May 2, 2024 11:37
1. React 프로젝트에서 src 폴더 내 파일들을 Next.js 프로젝트 src 폴더로 가져오기.
2. Next.js 에서 npm install 실행 => node_modules 재생성 
3. React 프로젝트에서 가져온 파일 중 landingPage.jsx 삭제, index.tsx 파일 수정
1. 디렉토리 이름을 더 직관적으로 변경 (folder => component-folder)
2. 일부 파일 이름을 더 직관적으로 변경
3. 모든 js, jsx 파일 확장자를 ts, tsx 로 변경
4. src 폴더 경로 @/ 로 모든 import 문 경로 수정
1. common/type/data-access-types.ts
2. 타입 파일의 모듈화 by 'tsconfig.json' 안에 module: force 옵션 추가
type: data-access 컴포넌트와 훅 타입 지정
@BaeUiJin BaeUiJin requested a review from kich555 May 17, 2024 13:13
@BaeUiJin BaeUiJin added 미완성🫠 죄송합니다.. 순한맛🐑 마음이 많이 여립니다.. labels May 17, 2024
@kich555 kich555 merged commit c5f8be3 into codeit-bootcamp-frontend:part3-배의진 May 17, 2024
Copy link
Collaborator

@kich555 kich555 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굿굿~~ 👍👍👍
ts에 점점 익숙해지시는게 눈에 보이네요!
앞으로도 이렇게만 계속 가주시면 될 것 같습니다 ㅎ

}) => {
const { userData } = useGetUser();
const { email, profileImageSource } = userData || {};
const profile = userData ? { email, profileImageSource } : null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게는 어떠세요?

const profile = useMemo(()=>{
if(! userData) return null;
const { email, profileImageSource, ...rest } = userData;
return { email, profileImageSource }
},[userData])

onKeyDown: (e: React.KeyboardEvent<HTMLElement>) => void;
}

export interface AlertModalProps extends ModalProps {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extends를 활용해 인터페이스를 확장하는거 좋네요!! 👍👍👍


export interface ShareModalProps extends ModalProps {
folderName: string;
onKakaoClick?: undefined; // TODO: 공유기능 구현 시 명확히 지정
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

주석 남겨두는것 좋습니다!

export const Cta = ({ children }) => {
type CtaProps = React.PropsWithChildren<{}>;

export const Cta: React.FC<CtaProps> = ({ children }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

children prop만 받을거라면 그냥

export const Cta: React.FC<React.PropsWithChildren> = ({ children }) => {

요렇게 하는게 괜찮지 않을까요?

interface IconAndTextButtonProps {
iconSource: string;
text: string;
onClick: () => void; // QUESTION : '() => setCurrentModal(modalId)'의 타입은 이게 맞을까? setter 함수의 타입은 여기서 고려해주지 않아도 되는걸까?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prop이름이 onClick이라면 onClick: () => void;이 아무래도 더 범용적인 타입이니 맞겠죠 ㅎ

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어찌되었든 함수가 param을 받지 않고 반환하는것이 아무것도 없다면 큰 의미에서는 void를 return하는게 맞으니까요 ㅎ

@@ -5,10 +5,12 @@ import { Modal } from "@/common/ui-modal";
import { ModalContentBox } from "@/common/ui-modal-content-box";
import { ModalContentButton } from "@/common/ui-modal-content-button";
import { ModalContentTitle } from "@/common/ui-modal-content-title";
import React from "react";
import { InputModalProps } from "../types/modal-prop-types";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

하나 추천이긴 한데
저는 import구문에서 type들은 type임을 보다 더 잘 표현하기 위해
import type { InputModalProps } from "../types/modal-prop-types"; 와 같이 사용합니다.

Copy link
Author

@BaeUiJin BaeUiJin May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모든 컴포넌트에서 수정완료!

interface InputProps {
type?: string;
value: undefined;
onChange: undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

왜 undefined일까요? 🧐

Copy link
Author

@BaeUiJin BaeUiJin May 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분에 주석 넣는 걸 깜빡했습니다. 아직 기능 구현을 안 해서 부모로부터 빈 값을 받고 있는 prop 이라서 일단 undefined 라고 했습니다. 😢

@@ -6,15 +6,23 @@ import { useBackgroundClick } from "@/common/util/useBackgroundClick";

const cx = classNames.bind(styles);

export const Popover = ({
type PopoverProps = React.PropsWithChildren<{
isOpen: Boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boolean이 아닌 Boolean 도 타입으로 인식하나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아.. 오타입니다. 대문자로 쓰니까 타입 별칭으로 인식합니다!

type PopoverProps = React.PropsWithChildren<{
isOpen: Boolean;
anchorRef: React.RefObject<HTMLElement>;
anchorPosition: { [position: string]: number }; // TODO: 맞는지 나중에 한번 더 확인할 것
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Record<string,number>를 하지 않은 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

단순히 몰라서 그랬습니다 😢 이 부분은 모두 Record<string, number> 로 수정했습니다!

ownerName: string | undefined;
folderName: string | undefined;
links: EditedSampleLink[] | undefined;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
      profileImage: string | undefined;
      ownerName: string | undefined;
      folderName: string | undefined;
      links: EditedSampleLink[] | undefined;
    }

Partial<{
      profileImage: string;
      ownerName: string;
      folderName: string;
      links: EditedSampleLink[];
    }>

이렇게 해도 좋겠네요 ㅎ

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

부모에게서 undefined 값이 넘어올까봐 이렇게 했습니다. 말씀해주신대로 타입 수정하고, 부모에서 prop 에 전달될 변수가 undefined 일 경우 아예 mapFolderData 가 호출되지 않도록 했습니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
미완성🫠 죄송합니다.. 순한맛🐑 마음이 많이 여립니다..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants